Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update project to Swift 5.10 and start on Sendable updates #833

Merged
merged 12 commits into from
Aug 7, 2024

Conversation

waltflanagan
Copy link
Member

No description provided.

@waltflanagan waltflanagan changed the base branch from main to 9.0.0 July 23, 2024 00:00
This was fun. `NSDictionary(buildSettings)` is not able to be compared because we wrap swift values so we must use `==` on the swift types since they are not concrete and equatable.
@@ -259,7 +259,7 @@ extension XCBuildConfiguration {
/// :nodoc:
func isEqual(to rhs: XCBuildConfiguration) -> Bool {
if baseConfigurationReference != rhs.baseConfigurationReference { return false }
if !NSDictionary(dictionary: buildSettings).isEqual(NSDictionary(dictionary: rhs.buildSettings)) { return false }
if buildSettings != rhs.buildSettings { return false }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasnt able to find info on how to run Sourcery to do generate this, but this makes tests pass. Happy to make sure the generated code is correct but will need an assist on running things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok. Let's make those changes manually.

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing job @waltflanagan. Regarding the APIs to obtain the objects referenced by the PBXObjectReference, feel free to tackle that in a separate PR.


// QUESTION: this is exposed to the project but so is `getThrowingObject` and `getObject`. What access patterns do we want to support?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think having three APIs for the same is unnecessary. I believe (it's been a long time) that the motivation for having getThrowingObject was to instruct developers at runtime about bad usages of the library. For example adding a reference to an object that's not strongly referenced by the project. However, looking at the getThrowingObject implementation, and don't know if the error that we are throwing is more helpful than trying to do a force unwrap of the referenced object.
I'm between making getThrowingObject errors more useful, which would require adding more context to PBXObjectReference to ensure it's in sync, which is a bit of a tricky challenge, or removing getObject and getThrowingObject and having a single accessor. I'd say that we do the latter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up going with object() -> T? since that preserved the most current callsites. Also added object(as: T.Type) to avoid a double cast when used in compactMap

@@ -259,7 +259,7 @@ extension XCBuildConfiguration {
/// :nodoc:
func isEqual(to rhs: XCBuildConfiguration) -> Bool {
if baseConfigurationReference != rhs.baseConfigurationReference { return false }
if !NSDictionary(dictionary: buildSettings).isEqual(NSDictionary(dictionary: rhs.buildSettings)) { return false }
if buildSettings != rhs.buildSettings { return false }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok. Let's make those changes manually.

@waltflanagan waltflanagan merged commit 9dd12c6 into 9.0.0 Aug 7, 2024
10 checks passed
@waltflanagan waltflanagan deleted the waltflanagan/Initial9.0Work branch August 7, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants